Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a sha256 implementation which is optimized for unconstrained runtime #9

Merged
merged 19 commits into from
Feb 4, 2025

Conversation

TomAFrench
Copy link
Member

Description

Problem*

Resolves #7

Summary*

Throwing this up to start this work. We'd need to add better testing, remove all the duplicated code and restructure the library before merging this.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench TomAFrench requested a review from kashbrti February 4, 2025 12:57
@TomAFrench TomAFrench marked this pull request as ready for review February 4, 2025 13:10
@TomAFrench TomAFrench requested a review from guipublic February 4, 2025 13:23
@TomAFrench
Copy link
Member Author

This should be good to go now. I've fixed the issue when building the benchmarks.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

Changes to circuit sizes

Generated at commit: 53a510c93618aded8f0a03c98257110f36687606, compared to commit: 8fd01aeb92ac6f635ddcbbf005184d3b0055d160

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
test_sha256_1.json -8 ✅ -0.54% -14 ✅ -0.11%
test_sha256_511.json -15 ✅ -0.09% -1,491 ✅ -2.38%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
test_sha256_200.json 7,312 (+4) +0.05% 32,892 (+4) +0.01%
test_sha256_512.json 16,334 (+4) +0.02% 64,892 (+4) +0.01%
test_sha256_1.json 1,481 (-8) -0.54% 13,156 (-14) -0.11%
test_sha256_511.json 16,473 (-15) -0.09% 61,129 (-1,491) -2.38%

src/sha256.nr Outdated Show resolved Hide resolved
let mut h: STATE = INITIAL_STATE;
// Pointer into msg_block on a 64 byte scale
for i in 0..num_full_blocks {
let (msg_block, _) = build_msg_block(msg, message_size, BLOCK_SIZE * i);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we don't use the msg_byte_ptr value, we can check to make sure that we're not unnecessarily calculating this value in build_msg_block for this case.

Copy link

@kashbrti kashbrti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/sha256.nr Show resolved Hide resolved
src/sha256.nr Show resolved Hide resolved
src/sha256.nr Show resolved Hide resolved
src/sha256.nr Show resolved Hide resolved
@TomAFrench TomAFrench merged commit a333d3c into main Feb 4, 2025
10 checks passed
@github-actions github-actions bot mentioned this pull request Feb 4, 2025
@TomAFrench TomAFrench deleted the tf/unconstrained-optimized-version branch February 4, 2025 15:34
@guipublic guipublic mentioned this pull request Feb 4, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Add brillig optimized implementation of sha256
2 participants